Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(argocd-apps)!: use maps instead of lists #2538

Merged
merged 12 commits into from
Mar 23, 2024
Merged

feat(argocd-apps)!: use maps instead of lists #2538

merged 12 commits into from
Mar 23, 2024

Conversation

irizzant
Copy link
Contributor

@irizzant irizzant commented Feb 21, 2024 •
edited
Loading

This PR fixes #2523

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@irizzant
feat(argocd-apps)!: use maps instead of lists
402af47 
Signed-off-by: irizzant <i.rizzante@gmail.com>
@irizzant
fix(chart): add artifacthub annotations
126bdf6 
Signed-off-by: irizzant <i.rizzante@gmail.com>
mkilchhofer
mkilchhofer requested changes Feb 29, 2024
View reviewed changes
Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost LGTM.

Are we confident that this solves all our issues? ;) @yu-croco @jmeridth are you also okay with this change?

charts/argocd-apps/Chart.yaml Outdated Show resolved Hide resolved
@jmeridth
Copy link
Member

jmeridth commented Mar 7, 2024

@mkilchhofer Looks good but wondering one thing:

range $k, $v := .Values.projects is in many place now but the $v (value) is not used in most of them. Should we prefer range $k, _ := .Values.projects instead or am I missing it?

@irizzant
fix: use the right range function
dfdc778 
Signed-off-by: irizzant <i.rizzante@gmail.com>
@irizzant
Copy link
Contributor Author

irizzant commented Mar 8, 2024

@jmeridth I updated the manifests to use range $k, _ :=

jmeridth
jmeridth reviewed Mar 8, 2024
View reviewed changes
Copy link
Member

@jmeridth jmeridth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple questions

charts/argocd-apps/Chart.yaml Outdated Show resolved Hide resolved
charts/argocd-apps/templates/applications.yaml Outdated Show resolved Hide resolved
charts/argocd-apps/templates/item-templates.yaml Outdated Show resolved Hide resolved
@irizzant
fix(argocd-apps): fix issues
06f9c71 
Signed-off-by: irizzant <i.rizzante@gmail.com>
@irizzant
Copy link
Contributor Author

irizzant commented Mar 20, 2024 •
edited
Loading

Any news ?

@jmeridth
Copy link
Member

jmeridth commented Mar 22, 2024 •
edited
Loading

@mkilchhofer if this passes I'm good with it. Looks like we need to fix something

@irizzant
Copy link
Contributor Author

@jmeridth is this on my end?

@jmeridth
Copy link
Member

@irizzant yes, something to do here. I'm afk currently, so can't look.

mkilchhofer
mkilchhofer requested changes Mar 23, 2024
View reviewed changes
Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didnt use the range function properly on all implemented resources.

$ helm template apps .
Error: parse error at (argocd-apps/templates/projects.yaml:1): range can only initialize variables

I fixed the project.yaml and run template again, then:

$ helm template apps .
Error: parse error at (argocd-apps/templates/item-templates.yaml:1): function "_" not defined

Use --debug flag to render out invalid YAML

I try to fix them

@mkilchhofer mkilchhofer requested a review from mbevc1 March 23, 2024 11:48
@mkilchhofer
Copy link
Member

I reverted the changes on the itemTemplates section which was contributed by @joelee2012 in PR:

Can you live with this @irizzant ? IMHO it is too complex to refactor this feature.

@mkilchhofer
fix: Use range function properly (revert complex changes on item-temp…
63c305c 
…lates)

Signed-off-by: Marco Maurer <mkilchhofer@users.noreply.github.com>
yu-croco
yu-croco approved these changes Mar 23, 2024
View reviewed changes
Copy link
Collaborator

@yu-croco yu-croco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 🚀

@irizzant
Copy link
Contributor Author

@mkilchhofer thank you for the refactoring.
The need for this PR comes for being able to override ArgoCD resources with helm value files, I'm not against leaving the itemTemplates out of this PR also because I see it uses nested lists.

@mbevc1 mbevc1 merged commit 237493a into argoproj:main Mar 23, 2024
6 checks passed
@irizzant irizzant deleted the 2523 branch March 23, 2024 16:56
@sathieu
Copy link
Contributor

sathieu commented Apr 8, 2024

@irizzant Probably a good opportunity to add schema?

@yu-croco yu-croco mentioned this pull request Jun 5, 2024
Closed
@hairmare hairmare mentioned this pull request Jun 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmeridth jmeridth jmeridth left review comments

@mkilchhofer mkilchhofer mkilchhofer approved these changes

@mbevc1 mbevc1 mbevc1 approved these changes

@yu-croco yu-croco yu-croco approved these changes

@pdrastil pdrastil Awaiting requested review from pdrastil

Assignees
No one assigned
Labels
argocd-apps size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make argocd-apps.applications a map instead of a list
6 participants
@irizzant @jmeridth @mkilchhofer @sathieu @mbevc1 @yu-croco